workflows/release-binaries: Stop using ccache #124415
Conversation
Using ccache is a potential security risk, because the GitHub Actions cache is writable by pull requests, which means that any GitHub user could upload malicious data to the cache.
|
@llvm/pr-subscribers-github-workflow Author: Tom Stellard (tstellar) ChangesUsing ccache is a potential security risk, because the GitHub Actions cache is writable by pull requests, which means that any GitHub user could upload malicious data to the cache. Full diff: https://github.com/llvm/llvm-project/pull/124415.diff 1 Files Affected:
diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml
index f9a264e7cf48f1..9e74610723f156 100644
--- a/.github/workflows/release-binaries.yml
+++ b/.github/workflows/release-binaries.yml
@@ -216,14 +216,6 @@ jobs:
id: setup-stage
uses: ./workflows-main/.github/workflows/release-binaries-setup-stage
- - name: Setup sccache
- uses: hendrikmuhs/ccache-action@ca3acd2731eef11f1572ccb126356c2f9298d35e # v1.2.9
- with:
- # Default to 2G to workaround: https://github.com/hendrikmuhs/ccache-action/issues/174
- max-size: 2G
- key: sccache-${{ runner.os }}-${{ runner.arch }}-release
- variant: sccache
-
- name: Configure
id: build
shell: bash
@@ -234,9 +226,7 @@ jobs:
${{ needs.prepare.outputs.target-cmake-flags }} \
-C clang/cmake/caches/Release.cmake \
-DBOOTSTRAP_LLVM_PARALLEL_LINK_JOBS=1 \
- -DBOOTSTRAP_CPACK_PACKAGE_FILE_NAME="${{ needs.prepare.outputs.release-binary-basename }}" \
- -DCMAKE_C_COMPILER_LAUNCHER=sccache \
- -DCMAKE_CXX_COMPILER_LAUNCHER=sccache
+ -DBOOTSTRAP_CPACK_PACKAGE_FILE_NAME="${{ needs.prepare.outputs.release-binary-basename }}"
- name: Build
shell: bash
run: |
|
carlocab
left a comment
carlocab
left a comment
There was a problem hiding this comment.
Seems fine to me, but note that GitHub Actions cache already has built-in protections against cache poisoning.
For example, caches from PR branches are quarantined from other branches so PRs generally cannot introduce malicious code into other PRs or onto the main branch until the PR is merged.
But we can merge this for now and revert when the cache has been audited properly.
|
@carlocab Is there some documentation about how the quarantining works? |
|
There's some documentation here: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache Also mentioned here, but with caveats: https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/ |
|
@carlocab OK, thanks for the documentation links. So it sounds like the cache is a little more secure than I thought. The benefit of using the cache is pretty low. It's only used for the stage1 builds, which is just a small part of the overall build time. So, I think it might be best to just not use it for the release binaries. In general, it's good practice to limit inputs into release artifacts and the cache is just another thing we need to try to secure. |
|
I've updated the commit message with a new justification for this change, since my previous message was misunderstanding how the caches work. |
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
Removing seems reasonable enough to me, but I'm also pretty sure this isn't a super fruitful attack vector given how PRs from forks I'm pretty sure push caches into the context of the fork.
Completely closing off the avenue is not a bid idea though.
|
/cherry-pick b32e55d |
|
/pull-request #125009 |
Using ccache relies on the GitHub Actions Cache, which may be susceptible to cache poisoning. See https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/ Even though these attacks may be difficult, it's better to err on the side of caution and ensure that the build environment for our releases is as isolated as possible. Additionally, ccache was only being used for the stage1 build, which is a small part of the overall build, so the speed up from using it was not that large. (cherry picked from commit b32e55d)
4 participants
Using ccache relies on the GitHub Actions Cache, which may be susceptible to cache poisoning. See https://adnanthekhan.com/2024/05/06/the-monsters-in-your-build-cache-github-actions-cache-poisoning/
Even though these attacks may be difficult, it's better to err on the side of caution and ensure that the build environment for our releases is as isolated as possible. Additionally, ccache was only being used for the stage1 build, which is a small part of the overall build, so the speed up from using it was not that large.